fix(renderer): align font metrics to device pixel boundaries to prevent seams#6
Conversation
…nt seams When devicePixelRatio is non-integer (e.g. 1.25, 1.5, 1.75 from browser zoom or HiDPI displays), rounding cell width/height to the nearest CSS pixel with Math.ceil() produces fractional *physical* pixel coordinates at cell edges. The canvas rasterizer antialiases clearRect/fillRect calls at those sub-pixel boundaries. With alpha:true on the canvas (enabled in coder#93 for transparent backgrounds), the resulting partially-transparent edge pixels composite against the page background and appear as thin black seams between rows and columns. Fix: round up to the nearest *device* pixel instead of CSS pixel. The +2/+1 paddings for glyph overflow stay in CSS units before the DPR multiplication so they scale correctly. Ports only the font-metrics subset of upstream PR coder#146 — the rest of that PR bundles a substantial render-loop refactor (startRenderLoop → scheduleRender) and several perf caches whose risk/benefit needs separate evaluation against our current architecture. Co-authored-by: tommyme <chris.b.you@qq.com> Inspired-by: coder#146
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the font metric calculations in CanvasRenderer to ensure that cell dimensions align with physical pixel boundaries, preventing anti-aliasing artifacts on displays with non-integer device pixel ratios. The review feedback suggests improving documentation consistency for the baseline property and highlights a potential issue where the cached devicePixelRatio could become stale if the browser zoom or display environment changes at runtime.
| baseline: number; // Distance from top to text baseline | ||
| width: number; // Character cell width in CSS pixels (multiple of 1/devicePixelRatio) | ||
| height: number; // Character cell height in CSS pixels (multiple of 1/devicePixelRatio) | ||
| baseline: number; // Distance from top to text baseline in CSS pixels |
There was a problem hiding this comment.
For consistency with the width and height properties, consider updating the comment for baseline to explicitly mention that it is also an integer multiple of the physical pixel size (1/devicePixelRatio).
| baseline: number; // Distance from top to text baseline in CSS pixels | |
| baseline: number; // Distance from top to text baseline in CSS pixels (multiple of 1/devicePixelRatio) |
| // | ||
| // The +2/+1 pixel paddings stay in CSS-pixel units before the DPR | ||
| // multiplication so the glyph-overflow margin scales correctly. | ||
| const dpr = this.devicePixelRatio; |
There was a problem hiding this comment.
The devicePixelRatio is captured during the constructor and stored in this.devicePixelRatio. However, browser zoom levels or moving the window between monitors with different DPIs can change window.devicePixelRatio at runtime. Since measureFont (and remeasureFont) uses the cached this.devicePixelRatio, the font metrics will become misaligned if the system DPR changes, causing the seams to reappear. Consider adding a mechanism to update this.devicePixelRatio when the resolution changes or refreshing it from window.devicePixelRatio within measureFont if it wasn't explicitly fixed in the constructor options.
1 participant
Summary
Fixes thin black seams that appear between cells at non-integer
devicePixelRatio(browser zoom 125/150/175%, HiDPI displays).Root cause: cell width/height were rounded to CSS pixels with
Math.ceil(), producing fractional physical pixel coordinates at cell edges. The canvas rasterizer antialiases there, and combined withalpha: true(added in coder#93 for transparency support), the resulting partially-transparent edge pixels composite against the page background as visible seams.Fix: round up to the nearest device pixel instead. Glyph-overflow paddings (+2/+1) stay in CSS units before the DPR multiplication so they scale correctly.
Scope difference vs upstream PR coder#146
Upstream #146 bundles four unrelated changes under a font-metrics title:
startRenderLoop→scheduleRender+ deferred write side-effects), and three perf caches (font, color, hovered-link-range identity). Those change rendering architecture and need separate evaluation/benchmarking against our current commits (feat(selection): Add triple-click and selection improvements coder/ghostty-web#115, fix: prevent terminal crash on resize during high-output programs coder/ghostty-web#132, feat: add mouse tracking support for terminal applications coder/ghostty-web#106 interminal.ts).Only the title-matching subset is ported here. The rest stays in
_tasks/upstream-ports/146-font-metrics-device-pixels.plan.mdfor a possible follow-up.Attribution
Thanks to @tommyme for the original implementation.
Test plan
bun run fmt && bun run lint && bun run typecheckbun test— 331 tests pass, 0 failbun run build:libbun run build:wasmnot run locally (Zig not installed); no WASM path touchedRisk
Low — surgical change inside
CanvasRenderer.measureFont. No public API surface touched. Render-loop architecture untouched (that part of upstream coder#146 deliberately not ported here).